Skip to content

@uppy/aws-s3: fix server generated keys not getting returned - #6498

Merged
qxprakash merged 16 commits into
mainfrom
fix/6496-server-generated-key
Sep 3, 2026
Merged

@uppy/aws-s3: fix server generated keys not getting returned #6498
qxprakash merged 16 commits into
mainfrom
fix/6496-server-generated-key

Conversation

@qxprakash

Copy link
Copy Markdown
Collaborator

fixes #6496

NO AI USED in code

we were always passing client generated keys from s3Opts methods ,

see :

in #onSuccess we're passing key returned from putObject

async #uploadNonMultipart(signal: AbortSignal): Promise<void> {
const { location, key } = await this.#options.s3Client.putObject({
key: this.#options.key,
data: this.#data,
fileType: this.#options.file.type || 'application/octet-stream',
metadata: this.#options.metadata,
onProgress: (bytesUploaded: number) => {
this.#chunkState[0].uploaded = bytesUploaded
this.#onProgress()
},
signal,
})
this.#onSuccess({
location,
key,
})
}

putObject directly returns the key it gets in params which is the client generated key

public override async putObject({
key,
data,
fileType = C.DEFAULT_STREAM_CONTENT_TYPE,
onProgress,
signal,
}: IT.PutObjectParams) {
this._checkKey(key)
const { xhr, url } = await this.request({
request: { method: 'PUT', key },
data,
onProgress,
signal,
contentType: fileType,
})
return {
location: U.removeQueryString(url),
etag: U.sanitizeETag(xhr.getResponseHeader('etag')),
key,
}
}

people can modify these keys on their signing backends, when a signing backend stores the object under a different key:

  • Single-part: upload succeeds, but upload-success reports a key that doesn't exist in the bucket. This is the reported issue in In @uppy/aws-s3 6.0, the key returned in file.response.body.key is the client-generated key, not the server-generated one #6496

  • Multipart: the reported key was correct (it's parsed from S3's CompleteMultipartUpload response), but the client key was used internally for every part/complete/abort request and persisted as s3Multipart.key for Golden Retriever. That only works if the server maps client→server key deterministically on every call; a server that generates a unique key fails at the first uploadPart with NoSuchUpload.

Fix

  • signRequest may now return { url, key }. request() resolves the key once (signedKey || request.key), and putObject / createMultipartUpload return it.
  • key is optional: signers that return only { url } get exactly the previous behaviour, so this is non-breaking. Servers that sign for a key other than the one requested must return it the plugin has no way to detect the change otherwise.

@qxprakash qxprakash self-assigned this Aug 26, 2026
@changeset-bot

changeset-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bb7af70

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@uppy/aws-s3 Minor
uppy Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@qxprakash
qxprakash marked this pull request as draft August 26, 2026 22:29
})

return { xhr, url }
return { xhr, url, signedKey: signedKey || request.key }

@qxprakash qxprakash Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the key your server signs for is not exactly the key it received, you must return it as key in the response. Otherwise Uppy assumes the object was stored under the key it requested.

This is the directive which we need add in our docs, sadly this wasn't the case previously, before the rewrite we used to return the key from the server

@qxprakash
qxprakash marked this pull request as ready for review August 26, 2026 22:56
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@qxprakash
qxprakash requested review from mifi and remcohaszing August 26, 2026 22:56
Comment thread packages/@uppy/aws-s3/src/s3-client/S3mini.ts
@qxprakash
qxprakash requested a review from mifi August 27, 2026 14:42
@mifi

mifi commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

i did an ai review: #6507

looks ok? then we can merge both

}
const params = new URLSearchParams({ method: req.method })
if (req.uploadId) params.set('uploadId', req.uploadId)
if (req.partNumber) params.set('partNumber', String(req.partNumber))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is req.partNumber? If it’s a number, you should check for nullishness instead of booliness. (0 is falsy.)

@qxprakash qxprakash Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that you mentioned it, I think we could also have typed it better, rather than any,

export interface UploadPartRequest extends PresignableRequestBase {
method: 'PUT'
uploadId: string
partNumber: number
}
, theoritically it can never be 0 at runtime since partnumbers are 1-based

@qxprakash qxprakash Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add non-null check for now

Comment thread packages/@uppy/aws-s3/src/s3-client/S3mini.ts
@qxprakash
qxprakash merged commit 41b3959 into main Sep 3, 2026
17 of 20 checks passed
@qxprakash
qxprakash deleted the fix/6496-server-generated-key branch September 3, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In @uppy/aws-s3 6.0, the key returned in file.response.body.key is the client-generated key, not the server-generated one

3 participants